-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
2D ensemble weights #231
2D ensemble weights #231
Conversation
I can't turn the PR into a draft, but you can wait a little before reviewing this. While updating the Notebooks, I realized that |
Alright, this is good for review. |
docs/notebooks/5_warminglevels.ipynb
Outdated
"</div>\n", | ||
"\n", | ||
"Next, the weights and the datasets can be passed to `xs.ensemble_stats` to calculate the ensemble statistics." | ||
"Next, the weights and the datasets can be passed to `xs.ensemble_stats` to calculate the ensemble statistics. Note that if `split_experiments=True` is used, the datasets should be split before being sent to `ensemble_statistics`." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this sentence. If we need to do it separatly doesn't that defeat the purpose ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You bring out a going counterpoint that even though we can now more easily weight the experiments separately, it's not really useful. For now, I've removed the text related to this from the notebooks, and split_experiments == False
is the default anyway.
Two options:
- I change it so that an equal weight is given to all experiments. i.e. if SSP585 has 30 simulations and SSP245 only has 5, the total weight is still 1 each. This would allow for some fancy ensemble statistics that are not possible if you compute each experiment separately.
- I keep it as it is, it just won't be too useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see when we would want 1... so I would vote for 2 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A case for 1 (which is kinda what I had in mind when designing the option) would be if you want to make statistics on the full scope of possible climate change. @sarahclaude will need to do that for her project (although in her case, we would need to allow the weight between experiments to differ from 1).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it will be useful to be able to associate weights to experiement in an ensemble! I am not using it directly in ensemble_stats.
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
# Conflicts: # HISTORY.rst
Pull Request Checklist:
number
) and pull request (:pull:number
) has been added.What kind of change does this PR introduce?
generate_weights
:independence_level: GCM
were changed substantially. See Incorrect weights for independence_level="GCM" #230.institution
split_experiments
: Pretty self-explanatoryskipna
:time
orhorizon
dimension. If the dimension is not the same across all datasets, they are first reindexed to cover allhorizon/time
.produce_horizon
so it can accept multiple periods or warming levels.cleanup
to prevent raising an error when nocat:
attribute exists in the datasets.xs.ensemble_stats
.xs.generate_weights
Does this PR introduce a breaking change?
FutureWarning
inensemble_stats
that was there sincexscen 0.4.0
.stats_kwargs
is not fully abandoned.independence_level: all
was renamedmodel
for better consistency.independence_level: GCM
were changed substantially. See Incorrect weights for independence_level="GCM" #230.period
inproduce_horizon
has been deprecated and replaced withperiods
.to_level
were updated to reflect more recent changes.Other information: